Skip to content

Update TaskQueue with changes from Windows repo#980

Merged
brianpepin merged 4 commits into
mainfrom
user/brianpe/fromwindows
May 19, 2026
Merged

Update TaskQueue with changes from Windows repo#980
brianpepin merged 4 commits into
mainfrom
user/brianpe/fromwindows

Conversation

@brianpepin

Copy link
Copy Markdown
Contributor

This PR is the result of a round trip between libhttpclient and the windows codebase. There are three areas of change:

  1. General cleanup / "fit" into windows codebase (no behavioral changes)
  2. Address very minor race condition during CAS operation
  3. Eat task queue submits during process detach

Changes

General Cleanup

I removed the HC_UNITTEST_API #ifdefs. In the Windows codebase the task queue library is built and then merged into either the runtime DLL or the unit test DLL. This means the same library needs to link to both (we don't build it twice). The code add for unit tests is small enough I think this is fine.

I also renamed SubmitPendingCallback to be plural (since now its behavior is any callback that is ready) and unwrapped the test-specific API cover fn.

CAS Race

In SubmitPendingCallbacks there was a very narrow window where the CAS is successfully exchanged and then the timer is set. There is a narrow gap here where the CAS could have been updated by another thread after this thread "wins" but before this thread changes the timer. My fix for this is to only return if the m_timerDue is still what we expect after the SetTimer call.

Process Detach

The PlayFab team and I were researching a game crash that looked like we were submitting a threadpool callback after the task queue handle was released. This turned out to be the wrong analysis. What was happening was the game was hitting some failure and calling ExitProcess while other parts of the game were still running. This ended up submitting task queue callbacks in late shutdown during process detach, and the Windows thread pool handles this by throwing an exception and crashing the process. The intent of this change is to stop masking the real problem with a process crash. This uses RtlDllShutdownInProgress, which while documented, is not exposed in any header. It does work if you just declare it and link to ntdll, but that causes a link time break for all consumers of libHttpClient since ntdll is not in the current linkage requirements. Instead, this code does a GetModuleHandle of ntdll (which is loaded into every process on Windows). We don't do this for UWP because GMH is not defined for that API set. We don't bother releasing the module handle because a) we don't want to re-acquire it on every task queue submit and b) it is never unloaded anyway, so a leaked reference doesn't matter.

Testing / Verification

  • All unit tests pass both in libHttpClient and Windows repos.
  • Verified xCode builds.
  • Deployed runtime to Xbox console and verified RtlDllShutdownInProgress code works correctly in that environment.

@brianpepin
brianpepin merged commit 91bbfaa into main May 19, 2026
15 checks passed
@jhugard

jhugard commented May 20, 2026

Copy link
Copy Markdown
Collaborator

@brianpepin: Good catch! I took a deep look at this last night and there are still two other, similar TOCTOU races between updating the next start time atomic and setting the OS timer with Start. I'll have a PR up later today with a fix that consolidates the logic in one method (I have the code written but need to verify before pushing).

@brianpepin

Copy link
Copy Markdown
Contributor Author

@brianpepin: Good catch! I took a deep look at this last night and there are still two other, similar TOCTOU races between updating the next start time atomic and setting the OS timer with Start. I'll have a PR up later today with a fix that consolidates the logic in one method (I have the code written but need to verify before pushing).

I'd like to take credit but an automated code review AI actually caught it :)

jhugard added a commit that referenced this pull request May 21, 2026
Follow-up to #975 and #980: the CAS-then-Start pattern for retargeting
m_timerDue had a TOCTOU window where thread A could win the CAS but
before calling Start(), thread B could CAS+Start an earlier deadline,
which thread A's Start() would then overwrite — stranding the earlier
callback until independent traffic arrived.

The fix in #980 added post-Start verification in SubmitPendingCallbacks,
but the same unguarded pattern existed in QueueItem and
PromoteReadyPendingCallbacks.

This change extracts the CAS+Start+verify logic into three helpers
(ArmTimerIfEarlier, ArmNextPendingCallback, RearmObservedDueTime) so
the post-Start verification is applied uniformly at every call site.
~67 lines of duplicated inline CAS logic removed.
jasonsandlin added a commit that referenced this pull request May 27, 2026
Follow-up to #975 and #980: the CAS-then-Start pattern for retargeting
m_timerDue had a TOCTOU window where thread A could win the CAS but
before calling Start(), thread B could CAS+Start an earlier deadline,
which thread A's Start() would then overwrite — stranding the earlier
callback until independent traffic arrived.

The fix in #980 added post-Start verification in SubmitPendingCallbacks,
but the same unguarded pattern existed in QueueItem and
PromoteReadyPendingCallbacks.

This change extracts the CAS+Start+verify logic into three helpers
(ArmTimerIfEarlier, ArmNextPendingCallback, RearmObservedDueTime) so
the post-Start verification is applied uniformly at every call site.
~67 lines of duplicated inline CAS logic removed.

Co-authored-by: Jason Sandlin <jasonsa@microsoft.com>
rgomez391 pushed a commit that referenced this pull request Jun 23, 2026
#975 rewrote the shared STL wait-timer backend (WaitTimer_stl.cpp) to replace
the O(N) pointer-keyed cancellation scan with an O(log N) per-timer generation
scheme: every Start() bumps a generation counter and pushes a fresh heap entry,
and the worker discards any popped entry whose generation no longer matches the
timer's current generation.

That scheme can drop a due callback. A Start() that races the worker between
its Peek() of the earliest entry and its dispatch of that entry bumps the
generation, so when the worker pops the entry it now reads a stale generation
and discards it instead of firing it. The re-pushed entry carries a new
deadline, so the original due callback is never delivered. With the entry gone,
the worker sleeps on the next (later or absent) deadline and goes idle.

On a single-port manual XTaskQueue this is unrecoverable: the platform HTTP
provider re-arms its poll via XTaskQueueSubmitDelayedCallback, and once that
re-poll is dropped there is no independent traffic to wake the queue and sweep
the pending list again. The symptom is an infinite sign-in / inventory hang
(the delayed re-poll sits in the pending list while the timer thread waits in
_Cnd_timedwait, never re-armed).

Why the arming-layer fixes did not catch or fix it:
- #975, #980 and #983 (#983 = "Centralize timer-arming to close TOCTOU race")
  all hardened the CAS->Start bookkeeping in TaskQueuePortImpl (m_timerDue) in
  TaskQueue.cpp. Those fixes are correct and are retained here. But they assume
  Start() reliably arms the backend for the published deadline. The generation
  backend violates that assumption by dropping the entry after Start(), so no
  amount of arming-layer verification can recover it.
- The defect only affects WaitTimer_stl.cpp. Win32/UWP use WaitTimer_win32.cpp
  (OS CreateThreadpoolTimer, no generation scheme) and are unaffected. The
  libHttpClient unit-test project compiles the Win32 backend, so the Windows
  suite stayed green and none of #975/#980/#983 exercised the STL backend.

Fix: restore the proven pre-#975 STL backend algorithm (libHttpClient 2.3.1,
commit 0fa5f24) - pointer-keyed cancellation and an unconditional notify on
every Set, which cannot discard a due entry - bridged to the post-#975 public
WaitTimer API (GetCurrentTime / GetDueTime / Start(dueTime)). The clock is
pinned to steady_clock (the pre-#975 code used high_resolution_clock, which is
steady_clock on libc++ but wall-clock system_clock on libstdc++); pinning keeps
the delayed-callback ordering monotonic on every platform that compiles this
backend, preserving #975's monotonic-time intent.

The arming-layer improvements in TaskQueue.cpp from #975/#980/#983 are kept
unchanged; only the STL timer backend is reverted.

Validated on-device in a shipping title: the reproducing infinite sign-in /
inventory hang no longer occurs with this backend and the current TaskQueue
arming logic.

Follow-up: the STL backend has no unit-test coverage (the test project compiles
only the Win32 backend). A dedicated STL-backend test lane is needed so a future
re-land of the generation optimization cannot reintroduce this class of strand.
@jasonsandlin
jasonsandlin deleted the user/brianpe/fromwindows branch July 9, 2026 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants